test: refactor and enhance P2 platform adapter tests#5359
test: refactor and enhance P2 platform adapter tests#5359whatevertogo wants to merge 3 commits intoAstrBotDevs:masterfrom
Conversation
- Simplify test_other_adapters.py using import-only tests - Update webchat, wecom, dingtalk, lark, slack tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
bot
added
the
size:XXL
Summary of ChangesHello @whatevertogo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the testing infrastructure for various P2 platform adapters. By introducing isolated test files and refactoring existing ones, the changes aim to reduce code duplication, enhance maintainability, and ensure consistent testing patterns across different messaging platforms. The update also includes a minor fix for the WebChat adapter's event handling, contributing to overall system stability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
dosubot
bot
added
the
area:platform
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我在这里给出了一些总体反馈：
- 针对 Lark、Slack、Dingtalk 和 Wecom 的基于子进程的测试，都在字符串中内嵌了大量几乎相同的 Python 代码；建议将通用的辅助逻辑（例如用于 stub SDK 模块、构建适配器以及运行用例的代码）抽取到共享的函数/模块中，以减少重复并提升可维护性。
- 在测试中嵌入很长的多行代码字符串，会让代码难以浏览和进行静态检查；可以考虑把这些 stub 移动到真实的 Python 模块中（例如放在
tests/stubs或一个共享的辅助模块下），这样编辑器和 linter 能更好地识别它们，未来的重构也会更加安全。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- The subprocess-based tests for Lark, Slack, Dingtalk, and Wecom all embed large chunks of almost-identical Python code as strings; consider extracting common helpers (e.g., for stubbing SDK modules, building adapters, and running cases) into shared functions/modules to reduce duplication and make them easier to maintain.
- Embedding long multi-line code strings inside tests makes them hard to navigate and statically inspect; you might consider moving these stubs into real Python modules (e.g., under a `tests/stubs` or shared helper) so that editors and linters can see them and future refactors are safer.帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈来改进后续的代码审查。
Original comment in English
Hey - I've left some high level feedback:
- The subprocess-based tests for Lark, Slack, Dingtalk, and Wecom all embed large chunks of almost-identical Python code as strings; consider extracting common helpers (e.g., for stubbing SDK modules, building adapters, and running cases) into shared functions/modules to reduce duplication and make them easier to maintain.
- Embedding long multi-line code strings inside tests makes them hard to navigate and statically inspect; you might consider moving these stubs into real Python modules (e.g., under a
tests/stubsor shared helper) so that editors and linters can see them and future refactors are safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The subprocess-based tests for Lark, Slack, Dingtalk, and Wecom all embed large chunks of almost-identical Python code as strings; consider extracting common helpers (e.g., for stubbing SDK modules, building adapters, and running cases) into shared functions/modules to reduce duplication and make them easier to maintain.
- Embedding long multi-line code strings inside tests makes them hard to navigate and statically inspect; you might consider moving these stubs into real Python modules (e.g., under a `tests/stubs` or shared helper) so that editors and linters can see them and future refactors are safer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This pull request refactors and enhances tests for P2 platform adapters (WebChat, Wecom, DingTalk, Lark, Slack) to improve test coverage and maintainability. The main goal is to simplify testing by using subprocess-based isolation for adapters with external SDK dependencies and implementing import-only tests for less critical adapters.
Changes:
- Simplified import-only tests for P2 platform adapters (QQ Official, WeChat Official Account, Satori, Line, Misskey, Wecom AI Bot)
- Added comprehensive subprocess-based tests for Wecom, DingTalk, Lark, and Slack adapters with mocked external dependencies
- Added unit tests for WebChat adapter using unittest.mock
- Fixed import statements in webchat_event.py to use correct internal paths
- Added stop_event attribute to WebChatAdapter for better test support
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_other_adapters.py | Import-only tests for P2 adapters (QQ Official, WeChat Official, Satori, Line, Misskey, Wecom AI Bot) |
| tests/unit/test_webchat_adapter.py | Unit tests for WebChat adapter covering initialization, metadata, and termination |
| tests/unit/test_wecom_adapter.py | Subprocess-based tests for Wecom adapter with mocked wechatpy dependencies |
| tests/unit/test_dingtalk_adapter.py | Subprocess-based tests for DingTalk adapter with mocked dingtalk_stream dependencies |
| tests/unit/test_lark_adapter.py | Subprocess-based tests for Lark adapter with mocked lark_oapi dependencies |
| tests/unit/test_slack_adapter.py | Subprocess-based tests for Slack adapter with mocked slack_sdk dependencies |
| astrbot/core/platform/sources/webchat/webchat_adapter.py | Added stop_event public attribute aliasing _shutdown_event |
| astrbot/core/platform/sources/webchat/webchat_event.py | Fixed imports to use correct internal module paths |
| "secret": "test_secret", | ||
| } | ||
|
|
||
| def test_adapter_import(self, platform_config, event_queue, platform_settings): |
There was a problem hiding this comment.
The test method references undefined fixtures 'event_queue' and 'platform_settings'. These fixtures are not defined anywhere in the test file. This will cause the tests to fail with a fixture not found error.
| "encoding_aes_key": "test_encoding_aes_key", | ||
| } | ||
|
|
||
| def test_adapter_import(self, platform_config, event_queue, platform_settings): |
There was a problem hiding this comment.
The test method references undefined fixtures 'event_queue' and 'platform_settings'. These fixtures are not defined anywhere in the test file. This will cause the tests to fail with a fixture not found error.
| "port": 5140, | ||
| } | ||
|
|
||
| def test_adapter_import(self, platform_config, event_queue, platform_settings): |
There was a problem hiding this comment.
The test method references undefined fixtures 'event_queue' and 'platform_settings'. These fixtures are not defined anywhere in the test file. This will cause the tests to fail with a fixture not found error.
| "channel_secret": "test_secret", | ||
| } | ||
|
|
||
| def test_adapter_import(self, platform_config, event_queue, platform_settings): |
There was a problem hiding this comment.
The test method references undefined fixtures 'event_queue' and 'platform_settings'. These fixtures are not defined anywhere in the test file. This will cause the tests to fail with a fixture not found error.
| "access_token": "test_token", | ||
| } | ||
|
|
||
| def test_adapter_import(self, platform_config, event_queue, platform_settings): |
There was a problem hiding this comment.
The test method references undefined fixtures 'event_queue' and 'platform_settings'. These fixtures are not defined anywhere in the test file. This will cause the tests to fail with a fixture not found error.
| "secret": "test_secret", | ||
| } | ||
|
|
||
| def test_adapter_import(self, platform_config, event_queue, platform_settings): |
There was a problem hiding this comment.
The test method references undefined fixtures 'event_queue' and 'platform_settings'. These fixtures are not defined anywhere in the test file. This will cause the tests to fail with a fixture not found error.
tests/unit/test_dingtalk_adapter.py
Outdated
|
|
||
|
|
||
| class TestDingtalkAdapterTypoCompatibility: | ||
| def test_send_with_sesisp_typo(self): |
There was a problem hiding this comment.
The test method name has a typo: "sesisp" should be "sesison" to match the actual case name "send_with_sesison_typo" being tested. While this doesn't affect functionality, it's confusing and inconsistent.
| def test_send_with_sesisp_typo(self): | |
| def test_send_with_sesison_typo(self): |
| "secret": "test_secret", | ||
| } | ||
|
|
||
| def test_adapter_import(self, platform_config, event_queue, platform_settings): |
There was a problem hiding this comment.
The test method references undefined fixtures 'event_queue' and 'platform_settings'. These fixtures are not defined anywhere in the test file or in a conftest.py file. This will cause the tests to fail with a fixture not found error. Either define these fixtures in this file (as done in test_webchat_adapter.py), create a conftest.py file with shared fixtures, or remove these unused parameters if they're not actually needed for import-only tests.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of refactoring and enhancing the tests for P2 platform adapters, improving isolation and coverage. The introduction of subprocess-based tests is a consistent and interesting approach for dependency isolation. The fix for the WebChat adapter shutdown signaling is also a welcome improvement.
I've left a few comments regarding some inconsistencies in import paths, which seem to be using legacy astrbot.api paths instead of the newer astrbot.core paths. Aligning these would improve code consistency and maintainability. I also found a minor typo in a test method name. Overall, these are solid changes that improve the test suite's quality.
| support_proactive_message=False, | ||
| ) | ||
| self._shutdown_event = asyncio.Event() | ||
| self.stop_event = self._shutdown_event |
There was a problem hiding this comment.
This change introduces a public alias stop_event for the private _shutdown_event. While this enables testing, it creates redundancy and can be slightly confusing to have two attributes pointing to the same event object. For better maintainability, consider a future refactoring to use a single, public attribute (e.g., stop_event) consistently throughout the class.
| from astrbot.api.message_components import Plain | ||
| from astrbot.core.message.message_event_result import MessageChain | ||
| from astrbot.core.platform.astr_message_event import MessageSesion | ||
| from astrbot.api.platform import MessageType |
There was a problem hiding this comment.
There are some import paths using the legacy astrbot.api module, while other parts of the codebase are migrating to astrbot.core. To improve consistency, consider updating these imports. For example:
from astrbot.api.message_components import Plainshould befrom astrbot.core.message.components import Plainfrom astrbot.api.platform import MessageTypeshould befrom astrbot.core.platform import MessageType
tests/unit/test_dingtalk_adapter.py
Outdated
|
|
||
|
|
||
| class TestDingtalkAdapterTypoCompatibility: | ||
| def test_send_with_sesisp_typo(self): |
There was a problem hiding this comment.
There's a typo in the test method name. It should be test_send_with_sesison_typo to match the case name being tested (send_with_sesison_typo) and improve readability.
| def test_send_with_sesisp_typo(self): | |
| def test_send_with_sesison_typo(self): |
| from astrbot.api.message_components import At, Image, Plain | ||
| from astrbot.api.platform import MessageType |
There was a problem hiding this comment.
The imports from astrbot.api seem to be using legacy paths. For consistency with the ongoing refactoring to astrbot.core, these should be updated. For example:
from astrbot.api.message_components import ...->from astrbot.core.message.components import ...from astrbot.api.platform import MessageType->from astrbot.core.platform import MessageType
| errors_mod.SlackApiError = SlackApiError | ||
| sys.modules["slack_sdk.errors"] = errors_mod | ||
|
|
||
| from astrbot.api.platform import MessageType |
There was a problem hiding this comment.
This import from astrbot.api seems to be using a legacy path. For consistency with the ongoing refactoring to astrbot.core, it should be updated to from astrbot.core.platform import MessageType.
tests/unit/test_webchat_adapter.py
Outdated
| await adapter.terminate() | ||
|
|
||
| # Verify stop_event is set after terminate | ||
| assert adapter.stop_event.is_set() |
| support_proactive_message=False, | ||
| ) | ||
| self._shutdown_event = asyncio.Event() | ||
| self.stop_event = self._shutdown_event |
test: add event queue and platform settings fixtures for testing test: fix typo in TestDingtalkAdapterTypoCompatibility test case
Simplify and enhance tests for P2 platform adapters (WebChat, Wecom, DingTalk, Lark, Slack) to improve test coverage and maintainability.
Modifications / 改动点
tests/unit/test_other_adapters.pyto use import-only tests (~80 lines removed)astrbot/core/platform/sources/webchat/webchat_adapter.pyandastrbot/core/platform/sources/webchat/webchat_event.pyBenefits:
Reduced code duplication
Easier to maintain
Consistent testing patterns across P2 platforms
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
通过修复 WebChat 关闭信号机制并全面重构适配器测试覆盖范围（使用隔离且依赖桩替身的冒烟测试），优化 P2 平台适配器。
Bug Fixes:
stop_event贯通 WebChat 适配器的关闭信号，确保terminate()能够正确向底层队列管理器发送信号。Tests:
Original summary in English
Summary by Sourcery
Refine P2 platform adapters by fixing WebChat shutdown signaling and overhauling adapter test coverage with isolated, dependency‑stubbed smoke tests.
Bug Fixes:
Tests: